RE1-T115 S3 fix by ucswift · Pull Request #354 · Resgrid/Core

ucswift · 2026-05-02T02:52:15Z

Summary by CodeRabbit

Bug Fixes
- Improved S3 storage error handling to gracefully manage malformed metadata responses. When metadata parsing fails, file existence checks now automatically fall back to alternative verification methods and retry transient failures for improved reliability.
Tests
- Added comprehensive test coverage for S3 storage error handling and fallback verification scenarios.

coderabbitai · 2026-05-02T02:52:22Z

📝 Walkthrough

Walkthrough

Added MSBuild properties to manage build output by platform, decorated test fixtures with [NonParallelizable], implemented presigned HTTP HEAD fallback in S3StorageService when S3 metadata calls throw FormatException, and extended related tests to cover the new fallback behavior.

Changes

Build Configuration & Test Parallelization

Layer / File(s)	Summary
MSBuild Properties `Directory.Build.props`	Configures shared build properties: `ResgridHostOS` defaults to `unix` but switches to `windows` on Windows platforms; `BaseIntermediateOutputPath` uses the host OS to segregate intermediates under `obj/$(ResgridHostOS)/`; `DefaultItemExcludes` extends to exclude `obj/**`.
Test Fixture Parallelization Control `Tests/Resgrid.Tests/Services/DepartmentSettingsServiceTtsLanguageTests.cs`, `Tests/Resgrid.Tests/Web/Tts/TtsConfigurationRegistrationTests.cs`, `Tests/Resgrid.Tests/Workers/Console/Tasks/TtsStaticPromptRefreshTaskTests.cs`	Four test fixtures are marked with `[NonParallelizable]` to prevent NUnit from running them in parallel with other tests.

S3StorageService Presigned HEAD Fallback

Layer / File(s)	Summary
Error Handling & Fallback Methods `Web/Resgrid.Web.Tts/Services/S3StorageService.cs`	`ExistsAsync` now catches `FormatException` from S3 metadata calls and delegates to new `ExistsWithPresignedHeadAsync`. Added `CreatePresignedHeadUrl(string objectKey)` to generate presigned S3 `HEAD` URLs. Added `ExistsWithPresignedHeadAsync(string objectKey, CancellationToken cancellationToken)` to execute an HTTP `HEAD` request against the presigned URL with retry logic, treating 404 as "not found" and retrying transient failures.
Fallback Behavior Tests `Tests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.cs`	New test `exists_async_should_fall_back_to_presigned_head_when_metadata_response_is_malformed` verifies `ExistsAsync` falls back when metadata throws `FormatException`, generates a presigned `HEAD` URL, executes exactly one HTTP `HEAD` request, and returns `true`. Updated `upload_async_should_fall_back_to_presigned_put_when_metadata_response_is_malformed` to support verb-specific presigned URLs (`HEAD` vs `PUT`), validates that both `HEAD` (returning 404) and `PUT` requests occur, and confirms `GetPreSignedURL` is called once per verb.

Sequence Diagram

sequenceDiagram
    actor Caller
    participant S3StorageService
    participant S3API as S3 Metadata API
    participant Logger
    participant PresignedURL as Presigned URL<br/>Generator
    participant HTTPClient as HTTP Client
    
    Caller->>S3StorageService: ExistsAsync(objectKey)
    
    S3StorageService->>S3API: GetObjectMetadataAsync()
    
    alt Metadata request throws FormatException
        S3API-->>S3StorageService: FormatException
        S3StorageService->>Logger: Log warning
        
        S3StorageService->>PresignedURL: CreatePresignedHeadUrl()
        PresignedURL-->>S3StorageService: presignedHeadUrl
        
        S3StorageService->>S3StorageService: ExistsWithPresignedHeadAsync()
        
        loop Retry loop
            S3StorageService->>HTTPClient: HEAD presignedHeadUrl
            HTTPClient-->>S3StorageService: Response (200, 404, or transient error)
            
            alt 200 OK
                S3StorageService-->>Caller: return true
            else 404 Not Found
                S3StorageService-->>Caller: return false
            else Transient Error & Retries Remaining
                S3StorageService->>S3StorageService: Wait & retry
            else Retries Exhausted
                S3StorageService-->>Caller: throw InvalidOperationException
            end
        end
    else Metadata succeeds
        S3API-->>S3StorageService: ObjectMetadata
        S3StorageService-->>Caller: return true/false
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

RE1-T115 S3 api fix #352: Adds presigned PUT fallback to UploadAsync in S3StorageService for the same malformed metadata scenario, complementing the presigned HEAD fallback added here for ExistsAsync.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'RE1-T115 S3 fix' references a ticket identifier but provides minimal context about the actual changes. While it relates to S3 storage fixes in the code, it lacks clarity on what the fix addresses.	Revise the title to be more descriptive, such as 'Handle FormatException in S3 metadata with HEAD request fallback' or similar, to clearly communicate the purpose of the change to future reviewers.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch develop

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Review rate limit: 0/1 reviews remaining, refill in 60 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

request-info · 2026-05-02T02:52:24Z

Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details?

ucswift · 2026-05-02T02:52:50Z

Approve

github-actions

This PR is approved.

coderabbitai

🧹 Nitpick comments (1)

Web/Resgrid.Web.Tts/Services/S3StorageService.cs (1)
242-251: 💤 Low value

Minor: Constant naming is slightly misleading.

PresignedPutUrlExpiryMinutes is used for HEAD URLs as well. Consider renaming to PresignedUrlExpiryMinutes for clarity, though this is a minor nitpick.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Web/Resgrid.Web.Tts/Services/S3StorageService.cs` around lines 242 - 251, The
constant name PresignedPutUrlExpiryMinutes is misleading when used for HEAD URLs
(see CreatePresignedHeadUrl); rename the constant to PresignedUrlExpiryMinutes
(or similarly neutral) and update all references (e.g., in
CreatePresignedHeadUrl and any presigned PUT methods) to use the new identifier,
updating its declaration and any XML/comments to reflect it represents expiry
for any presigned URL type to avoid confusion.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@Web/Resgrid.Web.Tts/Services/S3StorageService.cs`:
- Around line 242-251: The constant name PresignedPutUrlExpiryMinutes is
misleading when used for HEAD URLs (see CreatePresignedHeadUrl); rename the
constant to PresignedUrlExpiryMinutes (or similarly neutral) and update all
references (e.g., in CreatePresignedHeadUrl and any presigned PUT methods) to
use the new identifier, updating its declaration and any XML/comments to reflect
it represents expiry for any presigned URL type to avoid confusion.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 04d3b40f-04c5-45d2-8d5b-c44099a8a0f6

📥 Commits

Reviewing files that changed from the base of the PR and between b4b9e37 and 0b145ca.

📒 Files selected for processing (6)

Directory.Build.props
Tests/Resgrid.Tests/Services/DepartmentSettingsServiceTtsLanguageTests.cs
Tests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.cs
Tests/Resgrid.Tests/Web/Tts/TtsConfigurationRegistrationTests.cs
Tests/Resgrid.Tests/Workers/Console/Tasks/TtsStaticPromptRefreshTaskTests.cs
Web/Resgrid.Web.Tts/Services/S3StorageService.cs

RE1-T115 S3 fix

7c918b6

github-actions Bot approved these changes May 2, 2026

View reviewed changes

RE1-T115 More S3 fixes

0b145ca

coderabbitai Bot reviewed May 2, 2026

View reviewed changes

ucswift merged commit 21f36d7 into master May 2, 2026
18 of 19 checks passed

This was referenced May 2, 2026

RE1-T115 Fix #355

Merged

RE1-T115 TTS changes and fixed issue with SystemAuth api breaking som… #358

Merged

RE1-T115 TTS fixes #361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RE1-T115 S3 fix#354

RE1-T115 S3 fix#354
ucswift merged 2 commits intomasterfrom
develop

ucswift commented May 2, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 2, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Possibly related PRs

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

request-info Bot commented May 2, 2026

Uh oh!

ucswift commented May 2, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ucswift commented May 2, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Possibly related PRs

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

request-info Bot commented May 2, 2026

Uh oh!

ucswift commented May 2, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ucswift commented May 2, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 2, 2026 •

edited

Loading